-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fix #8642] Fix a false negative for Style/SpaceInsideHashLiteralBraces
when a correct empty hash precedes the incorrect hash
#8649
Conversation
1d50670
to
d17744b
Compare
This was a regression in 0.90 btw, it worked fine in 0.89. |
Did you manage to identify which commit introduced the regression? |
d17744b
to
41d2162
Compare
@bbatsov I think it's because of 6c05f69#diff-434a2611de47b29849cbf58aeb8094aeL141-R134 Before |
41d2162
to
40ae7e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regression is due to my mistake 💦 Thank you for the fixing. Can you rebase with the latest master branch?
…eralBraces` when a correct empty hash precedes the incorrect hash.
40ae7e9
to
6a3fab2
Compare
@koic of course! done. |
@dvandersluis Thanks! |
@koic @dvandersluis @bbatsov thank you for fixing this so quickly! would it be possible to cut a new release 0.91 with this fix kinda soon? it's fairly frustrating |
Fixes #8642.
I'm not 100% sure I understand what
ConfigurableEnforcedStyle
is doing (it'd be great if it could have more documentation added!), but the problem here is that the detected style was flipping betweenspace
andnospace
depending on which hash was being evaluated. The fix is to just pass the definedstyle
toambiguous_or_unexpected_style_detected
rather than trying to calculate it.Consider the following code (which is also added as a test):
ConfigurableEnforcedStyle
) isnil
. Because the spacing for the empty has is correct,correct_style_detected
is called, which setsdetected_as_strings
andupdated_list
to['space']
(since that is theEnforcedStyle
value).check
method, it detects an offense and runsincorrect_style_detected
. At this point, a space is requiredno_space
is passed toambiguous_or_unexpected_style_detected
. Because of this,updated_list
is set to the intersection of[space]
and[no_space]
, which is[]
, andno_acceptable_style!
is called. The offense gets registered (and corrected).no_acceptable_style?
returns true, andstyle_detected
short-circuits. This causesincorrect_style_detected
to return before adding an offense, and all further hash spacing issues are ignored in the file.My change is to not override style but pass in the style from config on both
correct_style_detected
andincorrect_style_detected
, which preventsno_acceptable_style?
from short-circuiting further offense detection. None of the other code or existing tests had to be changed.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists).master
(if not - rebase it).bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.